User Defined Action Instance Concurrency Limits #5287
Conversation
bdoyle0182
changed the title
Codecov Report
@@ Coverage Diff @@
## master #5287 +/- ##
==========================================
+ Coverage 76.50% 76.56% +0.05%
==========================================
Files 240 241 +1
Lines 14569 14625 +56
Branches 647 628 -19
==========================================
+ Hits 11146 11197 +51
- Misses 3423 3428 +5
... and 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I still can't decide on naming this action config |
| val actionLimit = actionMetaData.limits.containerConcurrency | ||
| .map(limit => | ||
| if (limit.maxConcurrentContainers > namespaceLimit) ContainerConcurrencyLimit(namespaceLimit) else limit) | ||
| .getOrElse(ContainerConcurrencyLimit(namespaceLimit)) |
There was a problem hiding this comment.
This ContainerConcurrencyLimit reminds me of intra-container concurrency.
Can we use ActionConcurrency instead? I think we need to consider changing the existing ActionConcurrency to something else like IntraConcurrency to make it more intuitive.
There was a problem hiding this comment.
I agree but that's going to be a breaking change for the existing users of it. Is there a way we could add another new field that maps to the same field stored in the db? I guess I just don't want to have to change the api.
I could rename it to ActionInstanceLimit? And rename the existing Limit case class to ActionIntraConcurrencyLimit.
However the api would have to remain the same such that the two fields the user supplies on the api would be
concurrency
instanceConcurrency
or I could add a third field to the api such that there's these field options on the action put
concurrency
intraConcurrency
instanceConcurrency
where if both intraConcurrency and concurrency are defined, concurrency is ignored and intraConcurrency takes precedence as the value that's used in the service. I could make it such that if you try to upload with both fields the api rejects, and any new uploads of actions with either field defined writes to intraConcurrency in the db and removes concurrency from the document if it exists.
thoughts?
There was a problem hiding this comment.
Yes, that sounds good to me.
So we can support both concurrency and intraConcurrency for a while and finally remove concurrency from API at some point.
The instanceConcurrency is a newly added field.
There was a problem hiding this comment.
Yep. I will get to cleaning up and renaming. Will let you know when I'm ready for another look
There was a problem hiding this comment.
@style95 so I went a little different. I changed the api fields and db fields to be:
concurrency
instances
I think this is sufficient in distinguishing the two mechanisms and they follow the same kind of verbiage for the api fields and keeps us from having to go through a whole api change process which I think can be avoided. I updated the code and documentation to refer to concurrency as intra concurrency including the swagger as well as the additions of this mr to InstanceConcurrency in code.
The concurrency and instances field go into the limits section of the actions document / api so it should be self explanatory that they represent a max and is explained in the documentation.
bdoyle0182
changed the title
* working prototype * consider when to turn on namespace throttling * tests and final cleanup * update swagger * fix container concurrency field * fix tests * renaming * update docs * more cleanup --------- Co-authored-by: Brendan Doyle <[email protected]> (cherry picked from commit 72bb2a1)
Description
Gives users the ability to configure action concurrency limits on their action with respect to their namespace concurrency limit. This finally allows users to be able to configure some level of fairness within their namespace. See POEM 4 for considerations and design.
Related issue and scope
My changes affect the following components
Types of changes
Checklist: